Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

TF-M PSA template: Lock Approtect in network core #17093

Merged

Conversation

MarkusLassila
Copy link
Contributor

@MarkusLassila MarkusLassila commented Aug 30, 2024

Add support for locking the Approtect in nRF53 network core, when TF-M transforms from provisioning life-cycle-state (LCS) to secure LCS.

Needs following changes:
nrfconnect/sdk-trusted-firmware-m#172
nrfconnect/sdk-mcuboot#330

Identified weak points that I am (especially) looking comments for:

  • PCD code partitioning.
    -> Comments received.
  • Usage of CONFIG_MCUBOOT_USE_ALL_AVAILABLE_RAM, which stores parts of the bootloader memory to non-secure.
    -> CONFIG_MCUBOOT_NRF_CLEANUP_NONSECURE_RAM, which clears the non-secure ram is set by SB_CONFIG_MCUBOOT_USE_ALL_AVAILABLE_RAM.

@github-actions github-actions bot added the changelog-entry-required Update changelog before merge. Remove label if entry is not needed or already added. label Aug 30, 2024
@MarkusLassila MarkusLassila changed the title Tfm psa template add network core update TF-M PSA template: Lock Approtect in network core Aug 30, 2024
@NordicBuilder
Copy link
Contributor

NordicBuilder commented Aug 30, 2024

CI Information

To view the history of this post, clich the 'edited' button above
Build number: 27

Inputs:

Sources:

trusted-firmware-m: PR head: 8c7fae3936da02b7db4f5c8aba174b252a2b326e
sdk-nrf: PR head: 27799307dbdc6d7a05320c8a36bdcb5f38fbc2e5
mcuboot: PR head: 68b96b802cdeef77ce4200e776afa46f6d3cfb66

more details

trusted-firmware-m:

PR head: 8c7fae3936da02b7db4f5c8aba174b252a2b326e
merge base: 899f0f54e76d41d70fac538f8a2d2cf171294a3b
Diff

sdk-nrf:

PR head: 27799307dbdc6d7a05320c8a36bdcb5f38fbc2e5
merge base: 214da5d26d11945473b56bf766e0854eb8ef3a63
target head (main): 07697de2eb4a0459672056c9159e4c593d9eb97a
Diff

mcuboot:

PR head: 68b96b802cdeef77ce4200e776afa46f6d3cfb66
merge base: 720fa02787366f9f787b847194f6814921147770
Diff

Github labels

Enabled Name Description
ci-disabled Disable the ci execution
ci-all-test Run all of ci, no test spec filtering will be done
ci-force-downstream Force execution of downstream even if twister fails
ci-run-twister Force run twister
ci-run-zephyr-twister Force run zephyr twister
List of changed files detected by CI (22)
bootloader
│  ├── mcuboot
│  │  ├── boot
│  │  │  ├── bootutil
│  │  │  │  ├── src
│  │  │  │  │  │ image_validate.c
│  │  │  ├── zephyr
│  │  │  │  ├── main.c
│  │  │  │  │ prj.conf
doc
│  ├── nrf
│  │  ├── releases_and_maturity
│  │  │  ├── releases
│  │  │  │  │ release-notes-changelog.rst
include
│  ├── dfu
│  │  ├── pcd.h
│  │  │ pcd_common.h
modules
│  ├── tee
│  │  ├── tf-m
│  │  │  ├── trusted-firmware-m
│  │  │  │  ├── platform
│  │  │  │  │  ├── ext
│  │  │  │  │  │  ├── target
│  │  │  │  │  │  │  ├── nordic_nrf
│  │  │  │  │  │  │  │  ├── common
│  │  │  │  │  │  │  │  │  ├── core
│  │  │  │  │  │  │  │  │  │  ├── native_drivers
│  │  │  │  │  │  │  │  │  │  │  │ spu.c
│  │  │  │  │  │  │  │  │  │  │ target_cfg.c
│  ├── trusted-firmware-m
│  │  ├── tfm_boards
│  │  │  ├── common
│  │  │  │  │ nrf_provisioning.c
│  │  │  ├── partition
│  │  │  │  │ region_defs.h
samples
│  ├── nrf5340
│  │  ├── netboot
│  │  │  ├── src
│  │  │  │  │ main.c
│  ├── tfm
│  │  ├── tfm_psa_template
│  │  │  ├── Kconfig.sysbuild
│  │  │  ├── README.rst
│  │  │  ├── boards
│  │  │  │  ├── nrf5340dk_nrf5340_cpuapp_ns.conf
│  │  │  │  │ nrf5340dk_nrf5340_cpuapp_ns.overlay
│  │  │  ├── sysbuild.conf
│  │  │  ├── sysbuild
│  │  │  │  ├── b0n
│  │  │  │  │  │ prj.conf
│  │  │  │  ├── mcuboot
│  │  │  │  │  ├── boards
│  │  │  │  │  │  ├── nrf5340dk_nrf5340_cpuapp.conf
│  │  │  │  │  │  │ nrf5340dk_nrf5340_cpuapp.overlay
subsys
│  ├── pcd
│  │  ├── Kconfig
│  │  ├── src
│  │  │  │ pcd.c
west.yml

Outputs:

Toolchain

Version: 3dd8985b56
Build docker image: docker-dtr.nordicsemi.no/sw-production/ncs-build:3dd8985b56_912848a074

Test Spec & Results: ✅ Success; ❌ Failure; 🟠 Queued; 🟡 Progress; ◻️ Skipped; ⚠️ Quarantine

  • ◻️ Toolchain - Skipped: existing toolchain is used
  • ✅ Build twister - Skipped: Skipping Build & Test as it succeeded in a previous run: 26
  • ✅ Integration tests
    • ✅ test-fw-nrfconnect-boot - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-chip - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-nrf-iot_cloud - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-nrf_crypto - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-tfm - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-fw-nrfconnect-zigbee - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-sdk-find-my - Skipped: Job was skipped as it succeeded in a previous run
    • ✅ test-sdk-sidewalk
    • ✅ test-sdk-mcuboot - Skipped: Job was skipped as it succeeded in a previous run
    • ⚠️ test-fw-nrfconnect-fw-update
Disabled integration tests
    • desktop52_verification
    • doc-internal
    • test_ble_nrf_config
    • test-fw-nrfconnect-apps
    • test-fw-nrfconnect-ble_mesh
    • test-fw-nrfconnect-ble_samples
    • test-fw-nrfconnect-fem
    • test-fw-nrfconnect-nfc
    • test-fw-nrfconnect-nrf-iot_libmodem-nrf
    • test-fw-nrfconnect-nrf-iot_lwm2m
    • test-fw-nrfconnect-nrf-iot_mosh
    • test-fw-nrfconnect-nrf-iot_nrf_provisioning
    • test-fw-nrfconnect-nrf-iot_positioning
    • test-fw-nrfconnect-nrf-iot_samples
    • test-fw-nrfconnect-nrf-iot_serial_lte_modem
    • test-fw-nrfconnect-nrf-iot_thingy91
    • test-fw-nrfconnect-nrf-iot_zephyr_lwm2m
    • test-fw-nrfconnect-proprietary_esb
    • test-fw-nrfconnect-ps
    • test-fw-nrfconnect-rpc
    • test-fw-nrfconnect-rs
    • test-fw-nrfconnect-thread
    • test-low-level
    • test-sdk-audio
    • test-sdk-dfu
    • test-sdk-pmic-samples
    • test-sdk-wifi
    • test-secdom-samples-public

Note: This message is automatically posted and updated by the CI

@NordicBuilder
Copy link
Contributor

You can find the documentation preview for this PR at this link. It will be updated about 10 minutes after the documentation build succeeds.

Note: This comment is automatically posted by the Documentation Publishing GitHub Action.

@NordicBuilder
Copy link
Contributor

NordicBuilder commented Aug 30, 2024

The following west manifest projects have been modified in this Pull Request:

Name Old Revision New Revision Diff
mcuboot nrfconnect/sdk-mcuboot@720fa02 nrfconnect/sdk-mcuboot@68b96b8 (main) nrfconnect/[email protected]
trusted-firmware-m nrfconnect/sdk-trusted-firmware-m@899f0f5 nrfconnect/sdk-trusted-firmware-m@8c7fae3 (main) nrfconnect/[email protected]

Note: This message is automatically posted and updated by the Manifest GitHub Action.

@MarkusLassila MarkusLassila force-pushed the tfm-psa-template-add-network-core-update branch 2 times, most recently from dd2e330 to d79b499 Compare September 3, 2024 07:56
@github-actions github-actions bot added the doc-required PR must not be merged without tech writer approval. label Sep 3, 2024
@MarkusLassila MarkusLassila force-pushed the tfm-psa-template-add-network-core-update branch from d79b499 to 9827f06 Compare September 3, 2024 08:03
@MarkusLassila MarkusLassila force-pushed the tfm-psa-template-add-network-core-update branch from 9827f06 to cdb44e8 Compare September 9, 2024 06:23
@Vge0rge
Copy link
Contributor

Vge0rge commented Sep 9, 2024

Overall this looks good to me, the only concern that I have is with the CONFIG_MCUBOOT_USE_ALL_AVAILABLE_RAM option. As long as we don't encrypt the firmware I don't see it as an issue but if we choose to encrypt the firmware is there any chance that this will leave the encryption key in the non secure memory in the end. Can someone from MCUBOOT comment on this?

@MarkusLassila
Copy link
Contributor Author

Overall this looks good to me, the only concern that I have is with the CONFIG_MCUBOOT_USE_ALL_AVAILABLE_RAM option. As long as we don't encrypt the firmware I don't see it as an issue but if we choose to encrypt the firmware is there any chance that this will leave the encryption key in the non secure memory in the end. Can someone from MCUBOOT comment on this?

@hakonfam: I think that you had some thoughts about this. Could you comment on this?

@MarkusLassila MarkusLassila removed the DNM label Oct 15, 2024
@MarkusLassila MarkusLassila force-pushed the tfm-psa-template-add-network-core-update branch from d39924c to 12a7f13 Compare October 15, 2024 08:46
@MarkusLassila MarkusLassila force-pushed the tfm-psa-template-add-network-core-update branch 4 times, most recently from 6fc0fba to e49d903 Compare October 16, 2024 11:30
@MarkusLassila MarkusLassila requested review from a team as code owners October 16, 2024 11:30
Copy link
Contributor

@umapraseeda umapraseeda left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving the changelog.

@MarkusLassila MarkusLassila force-pushed the tfm-psa-template-add-network-core-update branch from e49d903 to 0a89769 Compare October 18, 2024 08:50
Copy link
Contributor

@Vge0rge Vge0rge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@MarkusLassila
Copy link
Contributor Author

Ping @nrfconnect/ncs-co-build-system , @nrfconnect/ncs-pluto , @nrfconnect/ncs-code-owners

@MarkusLassila MarkusLassila force-pushed the tfm-psa-template-add-network-core-update branch from 0a89769 to aac12dd Compare October 21, 2024 07:06
samples/nrf5340/netboot/src/main.c Show resolved Hide resolved
while (!nrfx_nvmc_write_done_check())
;

pcd_done();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whilst not needed for this PR (or any future PRs), I assume this could fail to write e.g. if the device is purposely ran from too low a voltage that flash writing is available at or a clock/voltage glitch is used to skip the write, and it does not check that the write was successful before booting

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that your point holds. If you could pass the writing to the the UICR part, and still hit the pcd_done, it could be problematic. However, this particular piece of code should still be run by the manufacturer so that the device will transition form provisioning to secure life-cycle-state.

Add a PCD command for locking the Approtect in B0n.

Approtect will be locked from TF-M, which is separate from
Zephyr OS. For this purpose, the relevant parts of PCD
library are moved to a separate header, which can be used in
both TF-M and Zephyr.

If TF-M is used, delay locking of the PCD commands memory
area to be done in TF-M.

NCSDK-17920

Signed-off-by: Markus Lassila <[email protected]>
With nRF53, allow the network core Approtect to be locked from TF-M.

This is done when we are transitioning from provisioning LCS to
secure LCS.

NCSDK-17920

Signed-off-by: Markus Lassila <[email protected]>
Add support for updating network core with nRF5340.

External flash will be used for update images.

NCSDK-17920

Signed-off-by: Markus Lassila <[email protected]>
@MarkusLassila MarkusLassila force-pushed the tfm-psa-template-add-network-core-update branch from 15677ce to 2779930 Compare October 23, 2024 06:56
@NordicBuilder NordicBuilder removed the DNM label Oct 23, 2024
@rlubos rlubos merged commit 8bbcd7a into nrfconnect:main Oct 23, 2024
13 of 14 checks passed
@MarkusLassila
Copy link
Contributor Author

MarkusLassila commented Oct 23, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-required PR must not be merged without tech writer approval. manifest manifest-mcuboot manifest-trusted-firmware-m
Projects
None yet
Development

Successfully merging this pull request may close these issues.